-
Notifications
You must be signed in to change notification settings - Fork 276
Add 'agent: BoltAgent' listener argument #1437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1437 +/- ##
==========================================
+ Coverage 90.54% 90.59% +0.05%
==========================================
Files 222 225 +3
Lines 7129 7190 +61
==========================================
+ Hits 6455 6514 +59
- Misses 674 676 +2 ☔ View full report in Codecov by Sentry. |
…ependency AsyncBoltAgent imports AsyncWebClient which requires aiohttp. Eagerly importing it from the agent package __init__ breaks environments where aiohttp is not installed, since slack_bolt/__init__.py imports BoltAgent from this package. Follows the existing convention of not adding async module imports at the top level.
…7 compat Replace AsyncMock usage with coroutine-returning MagicMock wrappers, matching the pattern used in the sync test suite. This avoids the Python 3.8+ AsyncMock and the need for the mock backport package.
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments for the kind and knowledgeable reviewers!
| Experimental: | ||
| This API is experimental and may change in future releases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: We're using an "Experimental" warning while we developer this feature. Rather than working on a long-standing branch, we'd like to merge into main under a semver:patch then release a semver:minor when the experimental status is removed.
| FIXME: chat_stream() only works when thread_ts is available (DMs and threaded replies). | ||
| It does not work on channel messages because ts is not provided to BoltAgent yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: Important callout. I'd like to add ts support in a follow-up PR so that we can discuss the best approach.
| next_keys_required: bool = True, # False for listeners / middleware / error handlers | ||
| ) -> Dict[str, Any]: | ||
| all_available_args = { | ||
| all_available_args: Dict[str, Any] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: This fixed a linter warning
| if name == "args": | ||
| if isinstance(request, AsyncBoltRequest): | ||
| kwargs[name] = AsyncArgs(**all_available_args) # type: ignore[arg-type] | ||
| kwargs[name] = AsyncArgs(**all_available_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: The above fix allows us to remove this type ignore
WilliamBergamin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work 💯 this is already starting to take shape 🚀 left a few comments
One more thing around testing, what do you think about adding some unit tests in tests/slack_bolt(_async)/kwargs_injection/test_args.py that ensure build_required_kwargs only creates a BoltAgent when the agent keyword argument is present in required_arg_names
| if "agent" in required_arg_names or "args" in required_arg_names: | ||
| all_available_args["agent"] = request.context.agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clever 💯 I like this and wonder if we should follow this pattern for other keyword arguments 🚀
What do you think about including a warning here that informs developers that the agent is an experimental feature subject to change?
From what I can tell we should be able to do this by taking advantage of the FutureWarning like this
import warnings
class ExperimentalWarning(FutureWarning):
"""Warning for features that are still in experimental phase."""
pass
warnings.warn(
"agent is experimental and may change in future versions.",
category=ExperimentalWarning,
stacklevel=2
)IIUC every time a handler processes a request and uses the "agent" kwargs, this warning would be printed, this might be a bit annoying but it would be clear
| from .response import BoltResponse | ||
|
|
||
| # AI Agents & Assistants | ||
| from .agent import BoltAgent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 Maybe we should wait until the feature is GA before exporting it here?
Developers should still be able to import the class directly with something like
from slack_bolt.agent import BoltAgent
| if TYPE_CHECKING: | ||
| from slack_bolt.agent.async_agent import AsyncBoltAgent | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL: TYPE_CHECKING, it seems awesome and maybe we can use it elsewhere to improve our types checking for async stuff
IIUC this is to ensure that we
- Do not import
AsyncBoltAgentwheneverAsyncBoltContextis imported - Static type checking around
AsyncBoltAgentpasses AsyncBoltAgentis only imported whencontext.agentis invoked
| resolved_channel = channel or self._channel_id | ||
| resolved_thread_ts = thread_ts or self._thread_ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering here if falling back to the class instances channel_id, thread_ts, team_id and user_id is the best behavior 🤔
Like if a developer only passes one of these parameters and are unaware of the the fallback values they could end up chat streaming to the wrong location?
I'm not super familiar with chat_stream so this might be a non issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 i agree with @WilliamBergamin! channel_id and thread_ts seem to work as a pair so if a user changes one they should be aware they need to change the other. Maybe we throw a warning when one of these params is changed but not the other 🤔 ?
| def test_agent_chat_stream_overrides_context_defaults(self): | ||
| """Explicit kwargs to chat_stream() override context defaults.""" | ||
| client = MagicMock(spec=WebClient) | ||
| client.chat_stream.return_value = MagicMock(spec=ChatStream) | ||
|
|
||
| agent = BoltAgentDirect( | ||
| client=client, | ||
| channel_id="C111", | ||
| thread_ts="1234567890.123456", | ||
| team_id="T111", | ||
| user_id="W222", | ||
| ) | ||
| stream = agent.chat_stream( | ||
| channel="C999", | ||
| thread_ts="9999999999.999999", | ||
| recipient_team_id="T999", | ||
| recipient_user_id="U999", | ||
| ) | ||
|
|
||
| client.chat_stream.assert_called_once_with( | ||
| channel="C999", | ||
| thread_ts="9999999999.999999", | ||
| recipient_team_id="T999", | ||
| recipient_user_id="U999", | ||
| ) | ||
| assert stream is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about creating a tests.slack_bolt.agent and tests.slack_bolt_async.agent directory where unit tests dedicated to the BoltAgent like these ones can live?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed 🙌 it looks like we're mixing unit tests with full app scenarios 🤔 might be nice to follow up on this in another pr ⭐
srtaalej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments but its LGTM! thank you for bringing these changes in so quickly 🚀 ⭐ ⭐ ⭐
| def test_agent_chat_stream_overrides_context_defaults(self): | ||
| """Explicit kwargs to chat_stream() override context defaults.""" | ||
| client = MagicMock(spec=WebClient) | ||
| client.chat_stream.return_value = MagicMock(spec=ChatStream) | ||
|
|
||
| agent = BoltAgentDirect( | ||
| client=client, | ||
| channel_id="C111", | ||
| thread_ts="1234567890.123456", | ||
| team_id="T111", | ||
| user_id="W222", | ||
| ) | ||
| stream = agent.chat_stream( | ||
| channel="C999", | ||
| thread_ts="9999999999.999999", | ||
| recipient_team_id="T999", | ||
| recipient_user_id="U999", | ||
| ) | ||
|
|
||
| client.chat_stream.assert_called_once_with( | ||
| channel="C999", | ||
| thread_ts="9999999999.999999", | ||
| recipient_team_id="T999", | ||
| recipient_user_id="U999", | ||
| ) | ||
| assert stream is not None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed 🙌 it looks like we're mixing unit tests with full app scenarios 🤔 might be nice to follow up on this in another pr ⭐
Changelog
Do not include in our Changelog because this is marked as an experimental feature.
Summary
This pull request adds a
agent: BoltAgentlistener argument, which will be used to access AI agent-related features such aschat_streamand more.BoltAgentandAsyncBoltAgentclasses that provide a convenientchat_stream()method pre-configured with event context defaults (channel_id,thread_ts,team_id,user_id)kwargsinjection system so listeners can declare it as a parameter (e.g.def handle(agent: BoltAgent)) or access it viacontext.agentBoltAgentconstruction is deferred - only created when the listener actually requests the agent or args parameter, avoiding unnecessary object creation on every dispatchExperimental
The
agent: BoltAgentclass and listener argument are marked as an experimental feature. We are categorizing experimental features assemver:patchuntil the experimental status is removed.No exists tests were modified and the goal is to be a low risk feature to add to
main.Example
Known limitations
chat_stream()currently only works whenthread_tsis available in the event context (DMs and threaded replies). Top-level channel messages do not havethread_ts, andtsis not yet provided toBoltAgent- tracked as aFIXMEin the code. I've experimented with a solution, but I'd prefer to introduce it in a separate PR so that we can review it independently.Testing
→ Modified Sample App with
agent: BoltAgentandagent.chat_stream()Category
slack_bolt.Appand/or its core componentsslack_bolt.async_app.AsyncAppand/or its core componentsslack_bolt.adapter/docsRequirements
Please read the Contributing guidelines and Code of Conduct before creating this issue or pull request. By submitting, you are agreeing to those rules.
./scripts/install_all_and_run_tests.shafter making the changes.